[test] Reduce non-integration unit test runtime in ByteSync.Client.UnitTests#287
[test] Reduce non-integration unit test runtime in ByteSync.Client.UnitTests#287marwannettour wants to merge 8 commits intomasterfrom
Conversation
…to 50ms in PolicyFactoryTests, perf(tests): reduce CancellationToken timeout from 1000ms to 50ms in PolicyFactoryTests
…ewrite tests with TestScheduler
…onService to skip retry delays in tests
…wModel and override in tests
There was a problem hiding this comment.
Pull request overview
Reduces ByteSync.Client.UnitTests runtime by eliminating wall-clock waits in the slowest tests, using injectable timing hooks in production code and deterministic scheduling/overrides in tests.
Changes:
- Added injectable timing hooks in production (
ISchedulerfor Rx intervals, overridable delay method, configurable retry delay provider). - Rewrote multiple unit/integration tests to use virtual time / zero-delay retries / fast cancellation and to use per-test directories instead of global temp paths.
- Reduced RSA test iteration counts to cut CPU-heavy cryptography loops.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ByteSync.Client.UnitTests/ViewModels/TrustedNetworks/AddTrustedClientViewModelTests.cs | Uses a testable VM subclass to bypass UI delays; refactors VM creation in tests. |
| tests/ByteSync.Client.UnitTests/ViewModels/Sessions/DataNodes/DataNodeSourcesViewModelTests.cs | Switches selected path to a per-test directory via AbstractTester. |
| tests/ByteSync.Client.UnitTests/TestUtilities/Mine/TestRsa.cs | Lowers RSA loop iterations from 100 to 10 and updates related assertions. |
| tests/ByteSync.Client.UnitTests/Services/TimeTracking/TimeTrackingComputerTests.cs | Migrates time-based tests to TestScheduler virtual time (no real sleeps). |
| tests/ByteSync.Client.UnitTests/Services/Misc/Factories/PolicyFactoryTests.cs | Reduces cancellation timeout used to cut off retry waits. |
| tests/ByteSync.Client.UnitTests/Services/Inventories/PosixFileTypeClassifierTests.cs | Uses AbstractTester test directory and adds cleanup; simplifies temp cleanup logic. |
| tests/ByteSync.Client.UnitTests/Services/Inventories/InventoryLoaderIncompleteFlagTests.cs | Replaces Path.GetTempPath() usage with AbstractTester directory + cleanup. |
| tests/ByteSync.Client.UnitTests/Services/Inventories/FileSystemInspectorTests.cs | Uses AbstractTester directory for filesystem fixtures and centralizes cleanup. |
| tests/ByteSync.Client.UnitTests/Services/Configurations/LocalApplicationDataManagerTests.cs | Uses AbstractTester directory as temp root for filesystem-based tests. |
| tests/ByteSync.Client.UnitTests/Services/Comparisons/InventoryComparerPropagateAccessIssuesTests.cs | Switches temp directory creation to AbstractTester directory + cleanup. |
| tests/ByteSync.Client.UnitTests/Services/Comparisons/InventoryComparerIncompletePartsFlatTests.cs | Switches temp directory creation to AbstractTester directory + cleanup. |
| tests/ByteSync.Client.UnitTests/Services/Communications/Transfers/Downloading/SynchronizationDownloadFinalizerTests.cs | Uses AbstractTester directory for temp file paths used by zip/delta tests. |
| tests/ByteSync.Client.UnitTests/Services/Communications/ConnectionServiceTests.cs | Forces retry policy to use zero delays by setting the new delay provider hook. |
| tests/ByteSync.Client.IntegrationTests/Services/Communications/Transfers/R2UploadDownload_Tests.cs | Uses AbstractTester directory for integration test file roots/paths. |
| tests/ByteSync.Client.IntegrationTests/Services/Communications/Transfers/R2DownloadResume_Tests.cs | Uses AbstractTester directory and adds explicit cleanup in tear down. |
| src/ByteSync.Client/ViewModels/TrustedNetworks/AddTrustedClientViewModel.cs | Introduces DelayAsync virtual hook to replace hardcoded Task.Delay. |
| src/ByteSync.Client/Services/TimeTracking/TimeTrackingComputer.cs | Adds optional IScheduler to drive Observable.Interval deterministically in tests. |
| src/ByteSync.Client/Services/Communications/ConnectionService.cs | Adds optional retry-delay provider hook to bypass exponential backoff in tests. |
Comments suppressed due to low confidence (3)
tests/ByteSync.Client.UnitTests/Services/Configurations/LocalApplicationDataManagerTests.cs:27
CreateTestDirectory()is run for every test, but there is no[TearDown]to deleteTestDirectoryfor the tests that don’t deletetempRootthemselves (e.g., the first three tests). This will leaveByteSync_Automated_TestsGUID folders behind on dev/CI machines. Add a fixture-level[TearDown]that deletesTestDirectory(and remove per-test deletion oftempRootif it becomes redundant).
[SetUp]
public void SetUp()
{
CreateTestDirectory();
_environmentServiceMock = new Mock<IEnvironmentService>();
_environmentServiceMock.SetupGet(e => e.ExecutionMode).Returns(ExecutionMode.Regular);
_environmentServiceMock.SetupProperty(e => e.Arguments, []);
}
tests/ByteSync.Client.UnitTests/Services/Communications/Transfers/Downloading/SynchronizationDownloadFinalizerTests.cs:48
CreateTestDirectory()is called inSetup(), but the fixture’s[TearDown]only verifies mocks and never deletesTestDirectory. SinceAbstractTesterdoesn’t auto-cleanup, this will accumulate directories over time. Consider deletingTestDirectoryinTearDown(after verification), ideally in atry/finallyso cleanup still happens if verifications fail.
[SetUp]
public void Setup()
{
CreateTestDirectory();
_deltaManager = new Mock<IDeltaManager>(MockBehavior.Strict);
_temporaryFileManagerFactory = new Mock<ITemporaryFileManagerFactory>(MockBehavior.Strict);
_fileDatesSetter = new Mock<IFileDatesSetter>(MockBehavior.Strict);
_logger = new Mock<ILogger<SynchronizationDownloadFinalizer>>();
_sut = new SynchronizationDownloadFinalizer(
_deltaManager.Object,
_temporaryFileManagerFactory.Object,
_fileDatesSetter.Object,
_logger.Object);
}
[TearDown]
public void TearDown()
{
// Verify all expected (Verifiable) setups were called
_deltaManager.Verify();
_temporaryFileManagerFactory.Verify();
_fileDatesSetter.Verify();
}
tests/ByteSync.Client.IntegrationTests/Services/Communications/Transfers/R2UploadDownload_Tests.cs:56
CreateTestDirectory()is called in[SetUp], but[TearDown]only disposes_clientScopeand never deletesTestDirectory. This will leak user-profile test folders (created byAbstractTester) across integration test runs. Mirror the cleanup used inR2DownloadResume_Testsby deletingTestDirectoryinTearDown(guarding for null/exists).
[SetUp]
public void SetUp()
{
CreateTestDirectory();
// ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
if (ByteSync.Services.ContainerProvider.Container == null)
{
ServiceRegistrar.RegisterComponents();
}
_clientScope = ByteSync.Services.ContainerProvider.Container!.BeginLifetimeScope(b =>
{
b.RegisterType<CloudflareR2ClientFactory>().As<ICloudflareR2ClientFactory>().SingleInstance();
b.RegisterType<CloudflareR2Service>().As<ICloudflareR2Service>().SingleInstance();
b.Register(_ => GlobalTestSetup.Container.Resolve<Microsoft.Extensions.Options.IOptions<ByteSync.ServerCommon.Business.Settings.CloudflareR2Settings>>())
.As<Microsoft.Extensions.Options.IOptions<ByteSync.ServerCommon.Business.Settings.CloudflareR2Settings>>();
b.RegisterType<R2FileTransferApiClient>().As<IFileTransferApiClient>().SingleInstance();
});
// Set AES key for encryption/decryption used by SlicerEncrypter
using var scope = _clientScope.BeginLifetimeScope();
var cloudSessionConnectionRepository = scope.Resolve<ByteSync.Interfaces.Repositories.ICloudSessionConnectionRepository>();
cloudSessionConnectionRepository.SetAesEncryptionKey(AesGenerator.GenerateKey());
}
[TearDown]
public void TearDown()
{
_clientScope.Dispose();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [SetUp] | ||
| public void SetUp() | ||
| { | ||
| CreateTestDirectory(); | ||
| _dataNode = new DataNode { Id = "DN_1" }; | ||
| _sessionServiceMock = new Mock<ISessionService>(); |
There was a problem hiding this comment.
CreateTestDirectory() is called in [SetUp] but this test fixture never deletes TestDirectory. Since AbstractTester.CreateTestDirectory() creates a real folder under the user profile and does not auto-cleanup, this will leak directories across runs. Add a [TearDown] that deletes TestDirectory (guarding for null/exists) or stop using AbstractTester here if you only need a path string.
| vm.CancelCommand.Execute().Subscribe(); | ||
|
|
||
| // Wait until CanExecute becomes false | ||
|
|
||
| var sw = Stopwatch.StartNew(); | ||
| while (canExecuteCopy && sw.ElapsedMilliseconds < 1000) | ||
| { | ||
| Thread.Sleep(10); | ||
| } | ||
|
|
||
| canExecuteCopy.Should().BeFalse(); | ||
|
|
||
| // Complete the cancel to release IsExecuting | ||
|
|
||
| tcs.SetResult(); | ||
|
|
||
| // Wait until CanExecute becomes true again | ||
|
|
||
| sw.Restart(); | ||
| while (!canExecuteCopy && sw.ElapsedMilliseconds < 1000) | ||
| { | ||
| Thread.Sleep(10); | ||
| } |
There was a problem hiding this comment.
This test still uses real wall-clock polling (Stopwatch + Thread.Sleep(10) loops up to 1s twice). That undermines the PR goal (runtime reduction) and can be flaky under load. Prefer awaiting the CanExecute observable transitions directly (e.g., wait for the first false emission after starting CancelCommand, then the first true emission after completing the TCS), optionally with a timeout, instead of sleeping/polling.
| publicKeys.Count.Should().Be(10); | ||
| encryptedMessages.Count.Should().Be(10); | ||
|
|
||
| // We verify that the control we were applying works correctly | ||
| publicKeys.Any(pk => pk.SequenceEqual(publicKeys[50])).Should().BeTrue(); | ||
| encryptedMessages.Any(em => em.SequenceEqual(encryptedMessages[50])).Should().BeTrue(); | ||
| publicKeys.Any(pk => pk.SequenceEqual(publicKeys[5])).Should().BeTrue(); | ||
| encryptedMessages.Any(em => em.SequenceEqual(encryptedMessages[5])).Should().BeTrue(); |
There was a problem hiding this comment.
The final two assertions in this test are tautologies: a sequence will always contain the element at publicKeys[5] / encryptedMessages[5], so they don’t validate any behavior. Consider replacing them with a meaningful check (e.g., distinct count equals list count, or removing them since uniqueness is already asserted inside the loop).
| public Func<int, TimeSpan>? RetryDelaySleepDurationProvider { get; set; } | ||
|
|
||
| public async Task StartConnectionAsync() | ||
| { | ||
| var retryPolicy = Policy | ||
| .Handle<Exception>(ex => !(ex is BuildConnectionException bce && bce.InitialConnectionStatus == InitialConnectionStatus.VersionNotAllowed)) | ||
| .WaitAndRetryForeverAsync( | ||
| retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)), | ||
| retryAttempt => RetryDelaySleepDurationProvider?.Invoke(retryAttempt) ?? TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)), | ||
| (exception, _, _) => |
There was a problem hiding this comment.
RetryDelaySleepDurationProvider is a mutable public delegate that is read inside the Polly retry delay lambda. If the property is changed from another thread while StartConnectionAsync() is running, retry delays can change mid-loop, making behavior nondeterministic. Capture the provider in a local variable before building the policy (or make it immutable via constructor injection) so the retry policy uses a consistent delay function for the entire connection attempt.
| var policy = _factory.BuildFileUploadPolicy(); | ||
|
|
||
| using var cts = new CancellationTokenSource(); | ||
| cts.CancelAfter(1000); | ||
| cts.CancelAfter(50); | ||
|
|
There was a problem hiding this comment.
cts.CancelAfter(50) risks canceling the token before the policy has even executed the first attempt (e.g., under GC pauses / slow CI), which would skip the retry/log path and make this test flaky since it asserts the error log was emitted. Consider using a slightly larger timeout (still far below 1000ms), or triggering cancellation after observing the first retry/log event so the test remains deterministic.
| var policy = _factory.BuildFileUploadPolicy(); | ||
|
|
||
| using var cts = new CancellationTokenSource(); | ||
| cts.CancelAfter(1000); | ||
| cts.CancelAfter(50); | ||
|
|
There was a problem hiding this comment.
Same concern here: cts.CancelAfter(50) can fire before ExecuteAsync starts, which would prevent the retry handler from running and can make the log verification flaky. A slightly larger timeout or canceling only after the first retry is observed would keep the runtime low while improving reliability.
|
[test] Reduce non-integration unit test runtime in ByteSync.Client.UnitTests
Summary
ByteSync.Client.UnitTestsis the dominant contributor to non-integration test runtime. Several test classes rely on real wall-clock waiting thread sleeps,ManualResetEventtimeouts, and retry policy backoffs that accumulate into significant idle time per test run, slowing down local development feedback loops and PR validation.This PR addresses the five slowest test classes by applying targeted, minimal changes to production code (injectable timing hooks) and rewriting the affected tests to use deterministic, synchronous alternatives. The target is at least a 30% reduction in total runtime for the
ByteSync.Client.UnitTestsproject.Changes
1.
TestRsa— Reduce RSA iteration count (100 → 10)File:
tests/ByteSync.Client.UnitTests/TestUtilities/Mine/TestRsa.csEach of the four tests iterates 100 times over RSA key generation, encryption, and decryption. 10 iterations are entirely sufficient to prove key uniqueness and encryption correctness. The behavior under test does not change with a higher iteration count.
2.
PolicyFactoryTests— ReduceCancellationTokentimeout (1000ms → 50ms)File:
tests/ByteSync.Client.UnitTests/Services/Misc/Factories/PolicyFactoryTests.csThe retry tests verify that
BuildFileUploadPolicyretries on certain HTTP status codes by running until theCancellationTokenfires. The 1000ms timeout forces each of the 13 test cases to wait a full second. Since the only observable behavior under test is whether the policy enters the retry loop and logs the error, 50ms is sufficient: Polly captures the exception and starts its sleep (≥1s) within microseconds, and the cancellation interrupts that sleep.3.
TimeTrackingComputer+TimeTrackingComputerTests— InjectISchedulerFiles:
src/ByteSync.Client/Services/TimeTracking/TimeTrackingComputer.cstests/ByteSync.Client.UnitTests/Services/TimeTracking/TimeTrackingComputerTests.csTimeTrackingComputer.RemainingTimeis built onObservable.Interval(TimeSpan.FromSeconds(1)). The 22 tests rely on realThread.Sleep/ManualResetEvent.WaitOnecalls with timeouts of 1–3.5 seconds to observe interval ticks.Production change: an optional
IScheduler? scheduler = nullparameter is added to the constructor, defaulting toScheduler.Default. TheObservable.Intervalcall uses this scheduler.Test change: all 22 tests are rewritten to use a
TestScheduler(ReactiveUI.Testingv19.5.41 already referenced in the project). Virtual time is advanced with_scheduler.AdvanceBy(...), which runs synchronously with no real waiting.4.
ConnectionService+ConnectionServiceTests— Override retry sleep durationFiles:
src/ByteSync.Client/Services/Communications/ConnectionService.cstests/ByteSync.Client.UnitTests/Services/Communications/ConnectionServiceTests.csStartConnectionAsyncuses a PollyWaitAndRetryForeverAsyncpolicy with exponential backoff (2^nseconds). The retry test triggers 2 failures before success, causing real waits of several seconds.Production change: a
public Func<int, TimeSpan>? RetryDelaySleepDurationProviderproperty is added toConnectionService. When non-null, it replaces the default exponential formula in the retry policy. Production code never sets it.Test change:
SetUpsetsRetryDelaySleepDurationProvider = _ => TimeSpan.Zero, making the retry policy skip all inter-attempt delays.5.
AddTrustedClientViewModel+AddTrustedClientViewModelTests— VirtualDelayAsyncFiles:
src/ByteSync.Client/ViewModels/TrustedNetworks/AddTrustedClientViewModel.cstests/ByteSync.Client.UnitTests/ViewModels/TrustedNetworks/AddTrustedClientViewModelTests.csValidateClient()andRejectClient()each callawait Task.Delay(TimeSpan.FromSeconds(3))to display a transient success/error state. Several tests exercise these paths and wait the full delay.Production change: the
Task.Delaycalls are replaced withawait DelayAsync(...), whereDelayAsyncis aprotected virtualmethod defaulting toTask.Delay. This preserves production behavior while enabling test overrides.Test change: a private
TestableAddTrustedClientViewModelsubclass is introduced, overridingDelayAsyncto returnTask.CompletedTaskimmediately.CI Stability
All changes are designed to be fully deterministic in CI:
TestScheduler.AdvanceBy()operates in virtual time on the calling thread — unaffected by machine load.TimeSpan.Zeroin Polly translates toTask.Delay(0)— a synchronous yield, not a real sleep.Task.CompletedTaskis instantaneous by definition.CancellationTokentimeout inPolicyFactoryTestsprovides a large margin over the time needed for Polly to begin its sleep.No existing test behavior or assertion semantics are changed.